Added support for more test methods, including Yarn and Composer #71
Conversation
This also covers support for methods optionally taking a lock file.
| @@ -176,6 +176,15 @@ def _test(self, path, contents=None): | |||
| "encoding": "base64", | |||
| "files": {"target": {"contents": encoded}}, | |||
| } | |||
|
|
|||
| # Some test methods carry a second file, often a lock file | |||
| if additional: | |||
There was a problem hiding this comment.
Python is EAFP than LBYL therefore a Pythonic way of writing this code block would look like:
try:
read = getattr(additional, "read", None)
additional = additional.read()
encoded = base64.b64encode(additional.encode()).decode()
post_body["files"]["additional"] = {"contents": encoded}
except:
passI'm aware of the fact that the code block at the top of this method is LBYL and therefore this one is too, but worth noting IMHO
There was a problem hiding this comment.
I think I refer the consistency vs having two stanzas with different styles. Not against someone refactoring the whole method mind. It requires some additional type hints I think.
There was a problem hiding this comment.
You have a point there. let's take this one separately as part of a refactor (happy to take it)
| @@ -164,7 +164,7 @@ def notification_settings(self): | |||
| def invite(self, email: str, admin: bool = False): | |||
| raise SnykNotImplementedError # pragma: no cover | |||
|
|
|||
| def _test(self, path, contents=None): | |||
| def _test(self, path, contents=None, additional=None): | |||
There was a problem hiding this comment.
How about we add type hints for this method as well, like so:
def _test(self, path: str, contents: typing.IO = None, additional: typing.IO = None)using typing.IO will cover both binary(if necessary) and text file-like objects.
There was a problem hiding this comment.
This won't quite work as is, as the interface accepts file handles or strings.
There was a problem hiding this comment.
Thanks for clarifying, API looks ambiguous to me. shouldn't the function calling _test be the one reading the file content and passing that on? I guess this is an inherited design smell.
worth a re-factor, though I'm sure we'll have to consider backwards compatibility here.
for instance why test_pipfile(self, content) is acting like test_pipfile_content(self, content) while what I think would be cleaner is:
""" test_pipfile tests a pip file for issue """
def test_pipfile(self, pip_file: typing.TextIO ) -> bool:
with open(pip_file) as f:
self._test(contents=f.read())
| @@ -164,7 +164,7 @@ def notification_settings(self): | |||
| def invite(self, email: str, admin: bool = False): | |||
| raise SnykNotImplementedError # pragma: no cover | |||
|
|
|||
| def _test(self, path, contents=None): | |||
| def _test(self, path, contents=None, additional=None): | |||
There was a problem hiding this comment.
Thanks for clarifying, API looks ambiguous to me. shouldn't the function calling _test be the one reading the file content and passing that on? I guess this is an inherited design smell.
worth a re-factor, though I'm sure we'll have to consider backwards compatibility here.
for instance why test_pipfile(self, content) is acting like test_pipfile_content(self, content) while what I think would be cleaner is:
""" test_pipfile tests a pip file for issue """
def test_pipfile(self, pip_file: typing.TextIO ) -> bool:
with open(pip_file) as f:
self._test(contents=f.read())
Added support for more test methods, including Yarn and Composer
This also covers support for methods optionally taking a lock file.